-
-
Notifications
You must be signed in to change notification settings - Fork 179
Cancel requests when element is removed #1045
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| } | ||
|
|
||
| export type ActionContext = RuntimeContext & { | ||
| setCleanup: (fn: OnRemovalFn, key: string) => void // sets a cleanup function for this element and key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since actions can return values to callers, I needed to add a new context API that allow setting a cleanup based on a key. The key can't be auto-set to the action name because there could be different fetch actions used on the same element and we only want to keep the removalFn for the last fetch action.
|
I faced a problem today (see Discord thread) and it seems this change is addressing it. I wanted to bind an SSE connection to a particular view, and once that view is morphed away or replaced the SSE connection would reset. Currently it just keeps creating more SSE connections on every page transition. I kept wondering why my app is bugging out, turns out it was the multiple SSE connections patch-racing. |
|
@wmTJc9IK0Q Yes that is similar to my use case. I have an SSE endpoint that powers a specific server-side component and pushes updates to it. Whenever the parent component removes this child component I want the requests to stop and no more patching of elements. |
|
Maybe, for reasons I currently don't see, it makes sense to allow the old behavior through an option still: Detaching means it won't get canceled if But it definitely makes sense to make cancelation the new default behavior. If you want to keep an SSE stream triggered by an element, keep that element. Current behavior is rather counterintuitive and potentially dangerous (memory leaks, patch races). |
|
Thanks for the PR. We need to discuss internally if and how we want to handle this. |
|
@bencroker Anything I can do to move this forward? |
|
Not really. We've been busy and haven't come to a decision on this yet. |
|
@wmTJc9IK0Q since you’re requesting a breaking change in behaviour, can you please explain and justify the argument for it? Specifically, what problem is it supposed to solve, and what is preventing you from solving it using an |
|
I could really use this. Are there any work-arounds? |
|
If I may, consider a UI with a navigation bar and main display area such that clicking on a navbar element Approaching this from the other direction: Is the current behavior desirable? Are people really designing apps that depend on connections persisting once the element that launched it is gone? So, I like this PR and I think most people will want and expect this behavior. |
|
@bencroker I see two scenarios where this can be useful, depending on how the app is being architected: Single connection per pageIn the often "recommended" pattern of having a single connection for each page for all element and signal updates, if I want to navigate to a new page without a full page refresh, the old page connection needs to be closed or else invalid updates will keep getting pushed to the browser window and leave old requests open, leaking server and browser resources. This is the pattern I am currently using. As it stands right now, I have to do a full-page refresh which is slower and requires doing a lot of extra work to persist UI state. For example, I have global UI elements for filter state and a time range picker. I want that to persist while navigating through the app. So I have 3 options:
Component-specific connectionsAs my team grows on the project, I want to enable developers to write their own isolated "components". Some of those may have their own ways of loading data and one way to solve that could be to have them open their own SSE connections for fetching new data without touching the main page's SSE endpoint. In this case, if those components get removed from the page by a navigation or some change in a higher component in the main page's tree I would want the requests to be closed on those leaf elements. This case is more theoretical at this point for me. I was building things this way at first but was able to create a pattern on my backend for doing one request for the whole page that is working fine for async component updates right now. I have been using my datastar build from this PR in production for 2 months now and I have not encountered any issues with this behavior as of yet. I do have the same question as @goertzenator - I don't see how the existing behavior is at all desirable. I would add to that: what will break because of this? If anything this feels like a bug in the auto cancellation feature and I'd be surprised that anyone is relying on this behavior on purpose. If they are - I'd like to understand why. The auto cancel feature was also just recently added and I think now is the time to fix this bug before it goes stable. |
|
Edited option (1) about how local storage could be used instead of URL params, but this has it's own problems as well. |
|
Thanks for the PR and discussion! We’ve taken a different approach that has the same result, coming in RC.6. |
|
Ok, I’ll be glad if my desired behavior is there, but I don’t like being asked for a justification, spending time writing a very detailed response and then not getting any engagement back on that from you. So I wouldn’t call it a discussion really. I get being annoyed with thankless open source users but I’m an actual engaged dev committing some of my time and experience to your project. You are spending plenty of time answering random discord threads, so I don’t get the lack of engagement to someone who is actually reading and trying to improve your code. Guess that’s just not as fun as telling people you’re using it wrong? |
If you don't like justifying changes, this isn't the project for you.
We read your thoughts, thanked you and discussed internally the ramifications to you.... and everyone else. Your time caused engagement with the ideas and even a robust response that WE will support, not you.
No, they decided that making a solution that we feel comfortable was simpler and a better use of time than trying to coax this PR along. Your PR is a suggestion, not a mandate. Neat part of open source is you get zero say on how someone spends there time or PRs or Discord. If you don't like that fork and support your own approach. |
|
Released in v1.0.0-RC.6. |
This makes it so that when requestCancellation is
autoand the element that initiated the request is removed, the request will now be cancelled. Otherwise open SSE requests keep getting new events and patching elements that shouldn't be and resources are not released causing memory leaks.